[PM-25821] Migrate Cipher Admin operation API calls to SDK #560
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
==========================================
+ Coverage 78.59% 78.64% +0.05%
==========================================
Files 281 288 +7
Lines 29131 30635 +1504
==========================================
+ Hits 22895 24093 +1198
- Misses 6236 6542 +306 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nikwithak
force-pushed
the
vault/pm-25821/cipher-admin-ops
branch
from
aa8a96d to
7dc66a5
Compare
… vault/pm-25821/cipher-admin-ops
|
Claude finished @nikwithak's task —— View job Code Review CompleteTodo List
Overall Assessment: REQUEST CHANGESI've completed a comprehensive code review of PR #560. The code follows good architectural patterns and has strong test coverage, but there are 2 critical issues that must be fixed before merge. Critical Issues Found❌ Finding 1: Dead Code - Unused KeyStore instantiation in restore_manyLocation: The let store: KeyStore<KeyIds> = KeyStore::default();
#[allow(deprecated)]
let _ = store.context_mut().set_symmetric_key(
SymmetricKeyId::User,
SymmetricCryptoKey::make_aes256_cbc_hmac_key(),
);Fix: Remove lines 60-65 entirely. The function correctly uses the
|
|
Be mindful that the autogenerated API bindings do not have any graceful handling of malformed data. I.e. an enum outside the valid range errors out. https://bitwarden.atlassian.net/browse/PM-6169 tracks improvements to this, and if this improvement is required reach out to platform to ensure it's prioritized. CC @trmartin4 |
Hinton
left a comment
Hinton
left a comment
There was a problem hiding this comment.
I would consider moving all the admin commands into a separate client. While they share some common logic I think they fundamentally behave differently and it might be confusing for consumers to call an admin method and not have the state react?
CiphersClient & CiphersAdminClient
| /// Generate a new key for the cipher, re-encrypting internal data, if necessary, and stores the | ||
| /// encrypted key to the cipher data. | ||
| fn generate_cipher_key( | ||
| pub(crate) fn generate_cipher_key( |
There was a problem hiding this comment.
question: Is this change needed? It doesn't look like it's used outside this module.
There was a problem hiding this comment.
Originally I had re-used this for a separate admin file, but I ended up adding it to the same file. I thought I already removed this, will fix.
There was a problem hiding this comment.
I left this in (changed to pub(super) so it can be re-used by the CipherAdminClient since it shares the request types.
| repository: &R, | ||
| encrypted_for: UserId, | ||
| request: CipherCreateRequestInternal, | ||
| collection_ids: Vec<CollectionId>, |
There was a problem hiding this comment.
question: CipherCreateRequest has organization_id, is there a reason organization is part of the cipher create request while collections are part of the method? Collections are tightly coupled to an organization so this split does not make much sense.
There was a problem hiding this comment.
This object ends up mapping directly to the CreateCipherRequestModel, which is re-used across the different endpoints to create a cipher (POST /cipher, POST /cipher/create, POST /cipher/admin), even though POST /cipher does not take collection_ids. I will play around with ways to re-structure these in a way that fits what you're describing.
There was a problem hiding this comment.
I think we can just leave it for now, but would be nice to clean it up in the future.
| if as_admin && cipher_request.organization_id.is_some() { | ||
| cipher = api_client | ||
| .ciphers_api() | ||
| .post_admin(Some(CipherCreateRequestModel { | ||
| collection_ids: Some(collection_ids.into_iter().map(Into::into).collect()), | ||
| cipher: Box::new(cipher_request), | ||
| })) | ||
| .await? | ||
| .try_into()?; | ||
| } else if !collection_ids.is_empty() { | ||
| cipher = api_client | ||
| .ciphers_api() | ||
| .post_create(Some(CipherCreateRequestModel { | ||
| collection_ids: Some(collection_ids.into_iter().map(Into::into).collect()), | ||
| cipher: Box::new(cipher_request), | ||
| })) | ||
| .await | ||
| .map_err(ApiError::from)? | ||
| .try_into()?; | ||
| repository | ||
| .set(require!(cipher.id).to_string(), cipher.clone()) | ||
| .await?; | ||
| } else { | ||
| cipher = api_client | ||
| .ciphers_api() | ||
| .post(Some(cipher_request)) | ||
| .await | ||
| .map_err(ApiError::from)? | ||
| .try_into()?; | ||
| repository | ||
| .set(require!(cipher.id).to_string(), cipher.clone()) | ||
| .await?; |
There was a problem hiding this comment.
question: What's going on here? It doesn't really make much sense to have three different endpoints for creating ciphers.
- Is
ciphers/createnot identical to doing post onciphers? - From a REST perspective having a dedicated admin endpoint is also quite weird.
There was a problem hiding this comment.
The three endpoints is because of the way the backend is structured today.
POST /ciphersis only used for creating an individual cipher. Even though it accepts anorganization_idin the request, it does not have anywhere to addcollection_ids.POST /ciphers/createis how we create an org cipher, as it accepts thecollection_ids.- The admin endpoint has its own uses that do a separate set of permission checks than the standard endpoint.
There was a problem hiding this comment.
I updated this so that it only calls two endpoints (POST /ciphers or POST /ciphers/create), adn moved the third (/ciphers/admin) to a separate file / client.
| /// Creates a new [Cipher] and saves it to the server. | ||
| pub async fn create( | ||
| &self, | ||
| request: CipherCreateRequest, | ||
| ) -> Result<CipherView, CreateCipherError> { | ||
| self.create_cipher(request, vec![], false).await | ||
| } | ||
|
|
||
| /// Creates a new [Cipher] for an organization, and saves it to the server. | ||
| pub async fn create_org_cipher( | ||
| &self, | ||
| request: CipherCreateRequest, | ||
| collection_ids: Vec<CollectionId>, | ||
| ) -> Result<CipherView, CreateCipherError> { | ||
| self.create_cipher(request, collection_ids, false).await | ||
| } |
There was a problem hiding this comment.
The contract here is weird. CipherCreteRequest allows you to specify an organization id. That implies you can make orgnaizational items.
There was a problem hiding this comment.
I agree here - the CipherCreateRequest was built to match the valid fields on the API request model, but when we create an org cipher, the client calls the separate (POST /ciphers/create) endpoint, which takes the collection_Ids. The standard POST /ciphers endpoint does not have collection_ids in the response model.
I'm thinking of modifying the CipherCreateRequest to have both org_id and collection_ids, and writing the logic to call the POST /ciphers/create endpoint iff they are both present on the request; LMK your thoughts?
| use wiremock::{ | ||
| Mock, ResponseTemplate, | ||
| matchers::{method, path}, | ||
| }; |
There was a problem hiding this comment.
issue: We've stopped using wiremock for api mocking. You can look at crates/bitwarden-vault/src/folder/edit.rs for up to date exampels of using ApiClient::new_mocked.
There was a problem hiding this comment.
Updated - now uses standalone methods, and updated unit tests to test those standalones using ApiClient::new_mocked with passed-in dependencies.
|
|
||
| impl CipherEditRequest { | ||
| fn generate_cipher_key( | ||
| pub(crate) fn generate_cipher_key( |
There was a problem hiding this comment.
question: this doesn't seem used outside of this module.
| #[error(transparent)] | ||
| Decrypt(#[from] DecryptError), |
There was a problem hiding this comment.
question: What operation requires decrypt?
There was a problem hiding this comment.
Since the function input for these requests is plaintext, and the SDK handles encryption before sending to the server, we return the updated object decrypted as well. example
We return a CryptoError in other instances (e.g. the edit_cipher implementation here), but this call returns a DecryptError. I will look into casting it approriately to use the same error.
Tangentially: Do you think it makes sense to use this approach - returning the decrypted CipherView object to the consumer, rather than the encrypted Cipher (all of it locally only)
| // putCipherAdmin(id, request: CipherRequest) | ||
| // ciphers_id_admin_put | ||
| #[allow(missing_docs)] // TODO: add docs |
| #[error(transparent)] | ||
| Api(#[from] ApiError), |
There was a problem hiding this comment.
The methods putting Api errors in CipherError should really be updated to not use cipher error.
There was a problem hiding this comment.
The only one still using CipherError is the share operations, which calls existing functions that currently returns CipherError already (e.g. https://github.com/bitwarden/sdk-internal/blob/vault/pm-25821/cipher-admin-ops/crates/bitwarden-vault/src/cipher/cipher_client/share_cipher.rs#L180-L184) - I think we can migrate this one to its own error type in the future.
| edit: Default::default(), | ||
| favorite: Default::default(), | ||
| folder_id: Default::default(), | ||
| permissions: Default::default(), | ||
| view_password: Default::default(), | ||
| local_data: Default::default(), | ||
| collection_ids: Default::default(), |
There was a problem hiding this comment.
reflection: Putting default values here can be dangerous, because there is nothing to indicate the Cipher model is incomplete.
There was a problem hiding this comment.
Unfortunately several API operations don't return the full cipher, only the CipherMiniResponseModel - for the Admin operations, since we don't update the repository, the intent is to still return a consistent type the user can handle, if needed - the concern is valid though. I think we can:
- Merge it with the known existing Cipher data (if it's available),
- Continue with this approach, knowing it's primarily for the admin operations which don't update local state - if this route, I can move the logic to be private to the admin operations, rather than a blanket
Fromimplementation, - Return the
CipherMiniResponseModelas-is, or create aMiniCipherViewtype for mapping / returning,which doesn't expose these fields at all.
Do you have a preference? Feel free to ping me on Slack when you're free for a deeper discussion - there are a handful of API operations that currently only return a subset of data like this.
… vault/pm-25821/cipher-admin-ops
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-25821
📔 Objective
Migrates the logic for orchestrating API calls for several operations related to the Admin API functionality. Adds the following operations to
CiphersClient, which call the appropriate API endpoints:PR Notes: Sorry for the heft - The line count is high on this PR, but a majority of it is unit tests. This should have been split into multiple tickets, in hindsight.
Note also that this hasn't been tested directly with the client yet, and so additional changes may be needed if bugs are discovered when integrating into the clients (future tickets) - these operations are not currently used anywhere.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes